Report the task directives referenced by the task script - #7505
Report the task directives referenced by the task script#7505bentsherman wants to merge 1 commit into
Conversation
Collect the whole `task.` namespace in the script variable references, matching the legacy parser, and report them through TaskRun#isDirectiveReferenced so that an executor adjusting the resources at schedule time can tell whether the rendered command carries a value it is about to change. The names do not reach the task hash: `task` is a local variable of the task context, so TaskRun#getGlobalVars skips them. nf-seqera submits the affected tasks with prediction model `none`. Signed-off-by: Ben Sherman <bentshermann@gmail.com>
✅ Deploy Preview for nextflow-docs canceled.
|
|
Thanks for putting this together — I like the direction, and I agree the Pros over #7483, as I see it:
Blocker: no config coverage. This shape is common in nf-core/configs institutional profiles: process {
clusterOptions = { "-l h_vmem=${task.memory.toString().replaceAll(/[\sB]/, '')}" }
}
process { withName: FOO { ext.args = { "-Xmx${task.memory.toGiga()}g" } } }Here the script only shows Two smaller things: the stub isn't actually covered — |
|
I'm attempting to cover the config reference extending this PR. |
|
I have another option that we can try. Let me submit another PR |
|
Have you actually encountered the |
|
#7506 applies a fix at the runtime layer by simply recording accesses to task properties. That way we don't have to play whack-a-mole in the compiler |
|
The main config concern as pattern like this there are plenty in nf-core/configs |
|
Comparing the five approaches against the requirement: detect One fact collapses the matrix: a config value cannot reference
#7506 is the one I'd take. Observing the reads rather than inferring them gets the config Its one gap is the one it already documents: Two results from trying the alternatives:
One thing to carry into whichever lands: scope by source directive. |
|
But The But if you want to be extra sure then I would just go with #7506 . It should cover everything: script, config, v1/v2 parser |
|
Just saw your analysis. #7506 works for me. Feel free to test it / clean it up / merge it 👍 |
|
Cool, took your advice. Closing this in favour of #7506 |
Alternative to #7483
Collect the whole
task.namespace in the script variable references and report them throughTaskRun#isDirectiveReferenced()so that an executor adjusting the resources at schedule time can tell whether the rendered command carries a value it is about to change.The names do not reach the task hash:
taskis a local variable of the task context, soTaskRun#getGlobalVars()skips them.nf-seqera submits the affected tasks with prediction model
none.This PR covers references in
script:,shell:,stub:, andtemplatescripts. It does not cover references in process directives or config because I have not seen examples of such usage.